-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add fast path for single value in VALUES aggregator #130510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
61f3ad6
to
c1d8eb7
Compare
c1d8eb7
to
5860c6e
Compare
Hi @dnhatn, I've created a changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of collecting the first values into only the firstValues
array. So we only go to the hash if there's more than one value. I'll have another look in the morning. I think it's correct, but I'll need another scan to make sure.
@@ -90,6 +91,14 @@ public static void combineIntermediate(GroupingState state, int groupId, BytesRe | |||
|
|||
public static void combineStates(GroupingState current, int currentGroupId, GroupingState state, int statePosition) { | |||
BytesRef scratch = new BytesRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch can go below the quick bail outs.
for (int s = 0; s < selected.getPositionCount(); s++) { | ||
int group = selected.getInt(s); | ||
int count = -selectedCounts[group]; | ||
selectedCounts[group] = total; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the counts any more - it's the counts in values, which is one less than the actual number of values. That's tricky. I'm not entirely sure what's correct here.
Similar to #127849, this change adds an optimized path for leveraging ordinal blocks of intermediate input pages in the Values aggregator. Below are the micro-benchmark results. Before: ``` // 1 raw input page + 1000 intermediate input pages Benchmark (dataType) (groups) Mode Cnt Score Error Units ValuesAggregatorBenchmark.run BytesRef 1 avgt 2 0.382 ms/op ValuesAggregatorBenchmark.run BytesRef 1000 avgt 2 112.293 ms/op ValuesAggregatorBenchmark.run BytesRef 1000000 avgt 2 113182.908 ms/op ``` ``` After: // 1 raw input page + 1000 intermediate input pages Benchmark (dataType) (groups) Mode Cnt Score Error Units ValuesAggregatorBenchmark.run BytesRef 1 avgt 2 0.378 ms/op ValuesAggregatorBenchmark.run BytesRef 1000 avgt 2 34.410 ms/op ValuesAggregatorBenchmark.run BytesRef 1000000 avgt 2 64654.830 ms/op ``` 1K groups: 112 ms -> 34.4ms 1M groups: 113s -> 64s More to come with #130510 Relates #127849
This change introduces a fast path for the VALUES aggregator in the single-value case. For the first value seen in each group, we add it the new big array without touching the hash. For subsequent values, if they are the same as the current value, we skip them; if they differ, we trigger the slow path and add them to the hash. This optimization speeds up VALUES when the number of groups is large and most groups have only one value.
Before:
After: